feat: Add fixed-size array support to the Go code generator#8974
feat: Add fixed-size array support to the Go code generator#8974statxc wants to merge 8 commits intogoogle:masterfrom
Conversation
|
@jtdavis777 would you review this PR |
|
this is also a language I am not familiar with, and that I think we do not have an active maintainer of. so will take me some time |
5a05954 to
b9564b4
Compare
|
@jtdavis777 could you give me any updates for me? |
|
this looks like another chunky feature that will take me a little bit to digest -- what I can say is there is a |
…o/fixed-size-array-support
Good call on
Happy to close either gap if you want full parity. |
|
thanks for the explanation -- I didn't catch in my quick scan of your PR that you had already enabled. I don't think absolute parity is necessary out of the gate as long as basic functionality is present. I will do my best to review this (with some of my free time next week most likely) but I would also love to see if I can ping someone else familiar with flatbuffers Go to chime in |
|
@mustiikhalil do you have any Go experience? :) |
|
I am pretty sure there are some example FBS files that ensure that flatc can generate code for them and at least one of them is a dedicated arrays test. can you poke around and see if we can make sure Go is one of the languages tested with that? |
|
@jtdavis777 Good catch. I looked into it and there are two things I need to hook up:
I'll get both of those wired up. |
…o/fixed-size-array-support
33c56b7 to
c05dbb2
Compare
Updated! please review again @jtdavis777 |
|
test side LGTM - would still love to find another golang person to look this over |
|
@jtdavis777 I hope you had a great weekend. I want to merge this PR if it has no problem. It’s been two weeks since I submitted this PR.😉 |
|
@jtdavis777 Any updates for me, please. It has no problem now |
|
@jtdavis777 The CI testing is not relate to my changes. Please check again. |
|
@statxc yes unfortunately we have had a ci regression that is blocking merging anything. Hoping that will be fixed soon |
|
@jtdavis777 Could you please merge this PR? It have been a month since I submitted this PR? Why does it take a long? |
|
@statxc as I have explained elsewhere, I do not work for Google. I am a volunteer with very little time to dedicate to this repository, as much as I enjoy doing so. I also am not an expert in Go, or how it is used in and by flatbuffers, so I am not fully comfortable merging new language feature support without much scrutiny and verification that this is implemented properly and in a way that will work for all flatbuffers Go users. With what little time I have had, I have been focusing on requests which are much easier to digest and are in my areas of expertise. I am very thankful that you have taken the time to implement this feature, and am excited for it to be merged. But unless and until we get more support, pull requests like this will take a while to get reviewed and approved. I appreciate your patience and enthusiasm for contributing and hope you will continue to do so! |
|
I will close this PR because it took long time since I opened this PR. Not yet merged. |
Summary
Add fixed-size array support to the Go code generator, resolving a long-standing gap where Go was the only major language without support for the
[type:length]struct field syntax (fixes #8417).Changes
src/idl_parser.cpp: AddIDLOptions::kGoto theSupportsAdvancedArrayFeatures()allowlist.src/idl_gen_go.cpp: Implement array-aware code generation for Go, including:[N]Type)tests/go_arrays_test_test.go: Comprehensive test covering read, mutate, object API round-trip, and standalone struct creation against thearrays_test.fbsschema.tests/GoArraysTest.sh: Test runner script following the same pattern asGoTest.sh.Design
The implementation follows the same flattening strategy used by the Java and C# generators: nested struct array fields are expanded into per-sub-field slices in the
Createfunction signature. For example,d:[NestedStruct:2]becomesd_a [][]int32, d_b []TestEnum, d_c [][]TestEnum, d_d [][]int64. Field offsets were verified against the Java reference output.Test plan
go vetclean on generated codeTestArrayStructRead,TestArrayStructMutate,TestArrayStructObjectAPI,TestNestedStructStandalone)GoTest.sh) still passflatcbuilds with no warnings